-
Notifications
You must be signed in to change notification settings - Fork 6
Resolving bug in grb::dot with nonblocking backend
#398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Tests seem to pass without issue (we'll do full test suite after review). |
|
Hi @GiovaGa , I had a quick look -- assuming the fix is correct (haven't dived deep yet), isn't this line here also in error? |
|
The line you mention surely looks wrong, because it is the same index as the one below. But it may not be strictly related to the other fix |
|
I have now had a look again at this code, and I propose a new fix that is more convincing to me. I also fixed the line that you indicated. I will run tests to check everything works as expected |
|
And in fact tests failed. I will look into it |
|
After this fix tests pass on internal CI, except for |
|
Locally test pass. @anyzelman I think you can check that the change makes sense |
|
Note to self: ready for review |
b9051e9 to
9bd9f93
Compare
…from the left and right inputs). I extended the unit tests to try trip up the new implementation, but I appear to have not been able to construct a test that does-- oddly enough. Nevertheless, will keep the extended unit test
… nonzeroes of x, since that is the vector with guaranteed the fewest number of nonzeroes. Instead, the error was in the computation of the mask -- it used y instead of x. Special note that the other fix WAS correct-- so there were two bugs in total. Also a minor code style fix
|
Hi @GiovaGa -- could you check my latest commit? I think the issue was half-resolved, and should now be fully resolved. Can also discuss in person, perhaps easier |
|
All tests succeed (with LPF). CI running-- if pass, will merge. Concept release notes: The implementation of the dot-product for the nonblocking backend could iterate over different nonzero indices in its input vectors. This bug did not materialise always, and did not materialise if the dot-product was called with the dense descriptor. This MR extends the unit test of the dot product to test more challenging sparse nonzero patterns, especially patterns that differ amongst the inputs, and add tests with two vectors with partial overlap as well as with zero overlap. The thus-extended unit test was able to trigger the pre-existing bug, which this MR also addresses. In addressing the bug, furthermore, issue #408 was uncovered. Thanks to @GiovaGa for finding the bug, and for proposing an initial fix and unit test extension! |
|
@anyzelman Smoke test for simulated annealing-Replica Exchange (#378) fails with nonblocking backend (see Actions ) |
For now the branch contains just a test that reproduces the bug
Solves #397